-
Notifications
You must be signed in to change notification settings - Fork 104
feat(backend): store data to be transmitted to receiver in outgoing payments #3766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
sanducb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I only left a couple of questions.
| "Tenant ID of the outgoing payment." | ||
| tenantId: String | ||
| "Data to be transmitted to receiver." | ||
| dataToTransmit: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need dataToTransmit here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can indeed remove this from this resolver (I don't think the ASE needs to look it up necessarily).
| "Unique key to ensure duplicate or retried requests are processed only once. For more information, refer to [idempotency](https://rafiki.dev/apis/graphql/admin-api-overview/#idempotency)." | ||
| idempotencyKey: String! | ||
| "Data to be encrypted and sent to the receiver." | ||
| dataToTransmit: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though dataToTransmit is explicit enough for ourselves to understand it, I think that integrators might benefit from having it named something like senderData or senderDataToTransmit because of "sender's data" being a common phrase in a "payments" context. WDYT? CC @mkurapov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If "sender data" is an existing concept in payments, then I think senderData would work here as the field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanducb good q & I see your point but in the spirit of keeping it more generic, dataToTransmit works better I think. Even though it will usually be the sender's data (or more specifically, the sending customer's data) that is sent, the ASE can choose to pass in some other payment metadata in there
| CARD_SERVICE_URL: 'http://cloud-nine-wallet-card-service:3007' | ||
| CARD_WEBHOOK_SERVICE_URL: 'http://cloud-nine-wallet-card-service:3007/webhook' | ||
| DB_ENCRYPTION_SECRET: 'zO9KogehJECHReHgQr+ZWGkmgOD4AYa4ksUxALSwgM8=' | ||
| DB_ENCRYPTION_IV: 'e9jyNk0CKajCgI93Ga2v23R/1wGZ2lO339QRaOFgxHM=' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead generated a unique IV every time the data is signed and store it together with the data? Otherwise, I think we would get the same ciphertext given the same input/ dataToTransmit
| "Tenant ID of the outgoing payment." | ||
| tenantId: String | ||
| "Data to be transmitted to receiver." | ||
| dataToTransmit: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can indeed remove this from this resolver (I don't think the ASE needs to look it up necessarily).
| state: OutgoingPaymentState.Sending, | ||
| dataToTransmit: | ||
| deps.config.dbEncryptionSecret && dataToTransmit | ||
| ? encryptDbData(dataToTransmit, deps.config.dbEncryptionSecret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually quite like this, gives ASE option to encrypt if they choose to do so
| public getDataToTransmit(key?: string): string | null { | ||
| if (!this.dataToTransmit) return null | ||
| if (!key) return this.dataToTransmit | ||
| const { tag, cipherText, iv } = JSON.parse(this.dataToTransmit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key rotation might be a bit tricky here, since we could end up encrypting with one key, change the key, try to decrypt with the old one and fail. We can focus on this on a follow-up PR.
(maybe we allow to configure a list of keys instead?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Captured in RAF-1207.
Changes proposed in this pull request
DB_ENCRYPTION_SECRETand theDB_ENCRYPTION_IV, to be used in the symmetric encryption of sensitive columns in the databasedataToTransmitfield to the outgoing payment modeldataToTransmitfield to thedepositOutgoingPaymentLiquidityGraphQL resolverContext
Fixes RAF-1182 and fixes RAF-1179.
Checklist
fixes #numberuser-docslabel (if necessary)